[AMD] Optimize Kimi-K2.5-MXFP4 on MI355X: Enable AITER, Expert Parallel, and update to vLLM v0.18.0#936
Conversation
…pace - Remove redundant VLLM_ROCM_USE_AITER_MLA=1 and VLLM_ROCM_USE_AITER_MOE=1 (both default to True in vllm envs.py, only master switch needed) - Remove VLLM_ROCM_USE_AITER_TRITON_ROPE=1 (noop without --compilation-config custom_ops+=+rotary_embedding) - Switch VLLM_ROCM_QUICK_REDUCE_QUANTIZATION from INT8 to INT4 for better TTFT/TPOT (2.2x vs 1.17x per quickreduce benchmarks) - Add TP8EP1 back to all search spaces alongside TP4EP1 and TP4EP4 so InferenceX can sweep and determine optimal config empirically Made-with: Cursor
Apply `--no-enable-prefix-caching`
| if [ "$EP_SIZE" -gt 1 ]; then | ||
| EP=" --enable-expert-parallel" | ||
| else | ||
| EP=" " | ||
| fi |
There was a problem hiding this comment.
🟡 The new EP_SIZE check at line 47 uses single-bracket [ "$EP_SIZE" -gt 1 ], which emits [: : integer expression expected to stderr whenever EP_SIZE is empty or unset; fix with [ "${EP_SIZE:-0}" -gt 1 ] or [[ ${EP_SIZE:-0} -gt 1 ]].
Extended reasoning...
What the bug is: At kimik2.5_fp4_mi355x.sh:47, the newly added expert-parallel check uses POSIX single-bracket syntax. POSIX [ ] requires both operands of -gt to be non-empty integers; when EP_SIZE is empty or unset, bash prints '[: : integer expression expected' to stderr and exits the condition with a non-zero status, causing the else branch (no expert parallel) to be taken.
The specific code path: The new YAML search-space entries include configs without an ep field (e.g., { tp: 8, conc-start: 4, conc-end: 64 } and { tp: 4, conc-start: 4, conc-end: 64 }). For those runs, EP_SIZE is either not exported to the shell environment or is exported as empty. The check on line 47 fires and produces the stderr noise on every such invocation.
Why existing code does not prevent it: EP_SIZE is intentionally absent from the check_env_vars call at the top of the script, because it is genuinely optional - only the { tp: 4, ep: 4, ... } entry needs it. There is no set -e in the script, so execution continues to the else branch correctly, but the spurious diagnostic is still emitted.
Addressing the refutation: One verifier argues the CI sweep generator (generate_sweep_configs.py line 677) always supplies EP_SIZE=1 as a default when ep is absent, meaning the error never occurs in automated CI runs. This is plausible - if that default is reliably enforced by the pipeline, the bash error is never triggered in practice. However, the script is also invocable manually for local testing or ad-hoc runs where EP_SIZE may not be set, and the defensive pattern costs nothing to adopt.
Step-by-step proof (manual invocation):
- Set only the required env vars: MODEL=... TP=8 CONC=4 ISL=1024 OSL=1024 MAX_MODEL_LEN=... etc.
- Leave EP_SIZE unset.
- Run: bash kimik2.5_fp4_mi355x.sh
- Bash reaches line 47: [ "" -gt 1 ] emits 'bash: [: : integer expression expected' to stderr.
- Execution continues with EP=" " (no expert parallel) - correct behavior, but the spurious error was still emitted.
How to fix: Replace line 47 with: if [ "${EP_SIZE:-0}" -gt 1 ] (default to 0 when unset) or equivalently: if (( ${EP_SIZE:-0} > 1 )). Both handle empty or unset EP_SIZE silently without changing any observable behavior.
| version=`rocm-smi --showfw | grep MEC | head -n 1 | awk '{print $NF}'` | ||
| if [[ "$version" == "" || $version -lt 177 ]]; then | ||
| export HSA_NO_SCRATCH_RECLAIM=1 | ||
| fi |
There was a problem hiding this comment.
🟡 The MEC firmware version check at line 40 fails to set HSA_NO_SCRATCH_RECLAIM=1 when rocm-smi returns a non-numeric string (e.g., N/A): the empty-string guard is false, and bash arithmetic comparison -lt on a non-integer operand inside [[ ]] returns false, so the short-circuit OR is false overall and the safety flag is never set. Since HSA_NO_SCRATCH_RECLAIM=1 prevents crashes on older firmware, the safe default for an indeterminate version should be to set it; fix by adding a regex guard: [[ "$version" == "" || ! "$version" =~ ^[0-9]+$ || $version -lt 177 ]].
Extended reasoning...
Bug Analysis
The newly added MEC firmware version check at benchmarks/single_node/kimik2.5_fp4_mi355x.sh:39-42 reads:
version=$(rocm-smi --showfw | grep MEC | head -n 1 | awk '{print $NF}')
if [[ "$version" == "" || $version -lt 177 ]]; then
export HSA_NO_SCRATCH_RECLAIM=1
fiHow the Bug Manifests
The condition uses short-circuit OR with two branches: "$version" == "" handles the empty case, and $version -lt 177 handles the numeric comparison. A third case is unhandled: when version is a non-empty, non-numeric string such as N/A or unknown — common sentinels in ROCm tooling.
Inside bash [[ ]], the -lt operator performs arithmetic comparison. When the left-hand operand is a non-integer string, bash either silently coerces it to 0 or produces an error and returns false. The verifiers confirmed that for non-integer strings the comparison returns false in practice.
The Specific Code Path
rocm-smi --showfwoutputs a firmware table;grep MEC | head -n 1 | awk '{print $NF}'extracts the last field of the first MEC line.- If that field is
N/A,versionis set toN/A. [[ "N/A" == "" ]]evaluates to false (not empty).[[ N/A -lt 177 ]]— bash arithmetic on a non-integer returns false.- Short-circuit OR:
false || false→ false — theifbody is skipped. HSA_NO_SCRATCH_RECLAIMis never exported.
Why Existing Code Does Not Prevent It
The empty-string guard correctly covers the case where awk produces no output. It does not cover non-empty, non-numeric values. There is no type check or regex validation before using -lt on $version.
Impact
On systems where rocm-smi --showfw returns a non-numeric firmware version field, HSA_NO_SCRATCH_RECLAIM=1 will silently not be set. Per the script comment, this flag is needed on MEC firmware older than version 177 to prevent RCCL scratch-memory reclaim crashes. A machine with old firmware but a non-numeric version string in rocm-smi output would be left in the crash-prone configuration without any warning. The practical risk is low since rocm-smi output is normally numeric on production hardware, but the violated fail-safe assumption is a real defect.
Step-by-Step Proof
rocm-smi --showfwon a specific machine outputs a MEC line whose last field isN/A.versionis assigned the stringN/A.[[ "N/A" == "" ]]evaluates to false.[[ N/A -lt 177 ]]— bash arithmetic comparison with non-integer — evaluates to false.- Entire OR condition is false;
ifbody is not entered. HSA_NO_SCRATCH_RECLAIMremains unset, leaving potential for crash on older firmware.
Recommended Fix
if [[ "$version" == "" || ! "$version" =~ ^[0-9]+$ || $version -lt 177 ]]; then
export HSA_NO_SCRATCH_RECLAIM=1
fiThe added ! "$version" =~ ^[0-9]+$ clause ensures that any non-numeric (indeterminate) version string causes the safety flag to be set, correctly implementing the fail-safe default.
| if [[ "$version" == "" || $version -lt 177 ]]; then | ||
| export HSA_NO_SCRATCH_RECLAIM=1 | ||
| fi |
There was a problem hiding this comment.
🟡 The MEC firmware version check at line 41 fails silently when rocm-smi returns a non-numeric string (e.g., N/A): the arithmetic comparison $version -lt 177 errors in bash [[ ]] context, causing the overall OR condition to return false and leaving HSA_NO_SCRATCH_RECLAIM unset. Since the comment explains this flag prevents crashes on older/unknown firmware, the safe default for indeterminate versions should be to set the flag — add a regex guard like ! "$version" =~ ^[0-9]+$ as the middle condition.
Extended reasoning...
The firmware version check is intended to protect against crashes on older MEC firmware by setting HSA_NO_SCRATCH_RECLAIM=1. The check reads:
version=`rocm-smi --showfw | grep MEC | head -n 1 | awk '{print $NF}'`
if [[ "$version" == "" || $version -lt 177 ]]; then
export HSA_NO_SCRATCH_RECLAIM=1
fiHow the bug manifests: If rocm-smi --showfw outputs a non-numeric last field for the MEC line — for example N/A, unknown, or a version string with a non-numeric suffix — the empty-string guard passes (version is not empty), but the second operand $version -lt 177 attempts a bash integer arithmetic comparison on a non-integer. Inside [[ ]], this silently returns false (bash evaluates it as an error/false rather than raising a fatal error). Due to short-circuit OR semantics, the overall condition is false, so HSA_NO_SCRATCH_RECLAIM is never exported.
Why existing code doesn't prevent it: The empty string check "$version" == "" only handles the case where awk produces no output. It does not guard against non-integer strings like N/A that awk could reasonably extract as the last field of rocm-smi output. Bash integer comparison $var -lt N inside [[ ]] does NOT raise an error that propagates — it simply evaluates to false, making the entire OR condition false.
Concrete proof: Suppose rocm-smi --showfw outputs a line like ... MEC ... N/A. After awk '{print $NF}', version=N/A. Then:
[[ "N/A" == "" ]]→ false (short-circuit does NOT skip second condition)[[ N/A -lt 177 ]]→ bash arithmetic error, condition evaluates to false- Overall
[[ false || false ]]→ false HSA_NO_SCRATCH_RECLAIM=1is NOT set
The machine may then experience the RCCL memory reclaim crashes the flag was meant to prevent.
Impact: When firmware version is indeterminate (non-numeric), the workaround is silently skipped. The safe/fail-safe behavior should be to apply the workaround when the version cannot be confirmed to be ≥ 177.
Fix: Add a numeric guard as the middle condition:
if [[ "$version" == "" || ! "$version" =~ ^[0-9]+$ || $version -lt 177 ]]; then
export HSA_NO_SCRATCH_RECLAIM=1
fiThis ensures: empty string → set flag, non-numeric string → set flag, numeric < 177 → set flag, numeric ≥ 177 → do not set flag.
…default Made-with: Cursor
Head branch was pushed to by a user without write access
|
/sweep test-config --config-files .github/configs/amd-master.yaml --runner-config .github/configs/runners.yaml --config-keys kimik2.5-fp4-mi355x-vllm |
|
@seungrokj Kicking off a sweep. Run: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/23483872729 |
Summary
Supersedes #922 with review feedback addressed.
VLLM_ROCM_USE_AITER=1,VLLM_ROCM_QUICK_REDUCE_QUANTIZATION=INT4)--enable-expert-parallelwhenEP_SIZE > 1, and add correspondingtp: 4, ep: 4search-space entriesv0.16.0tov0.18.0for extra perf gains (addresses @seungrokj's review comment)--no-enable-prefix-cachingflag (addresses @seungrokj's review comment)--gpu-memory-utilizationfrom 0.95 to 0.90 for stability, change--block-sizefrom 64 to 1HSA_NO_SCRATCH_RECLAIM=1on older firmware (< 177) to avoid crashesChanges from original PR #922
VLLM_ROCM_USE_AITER=1enables all sub-modules automatically)VLLM_ROCM_QUICK_REDUCE_QUANTIZATIONfromINT8toINT4to match MXFP4 quantization--no-enable-prefix-cachingper reviewer requestvllm/vllm-openai-rocm:v0.18.0per reviewer requestChanged Files
.github/configs/amd-master.yamlbenchmarks/single_node/kimik2.5_fp4_mi355x.shTest Plan